Repository navigation
Conversation
|
I am suprised to see all these segmentation faults. In my testing everything seemed to work nicely but I guess it was just crashing silently and working properly. I will need to modify functionCall() more heavily to not send nullptr to the library functions. This kinda undermines my idea of reusing functionCall for anonymous functions but I still think it is better than duplicating the function. |
|
I switched from the makefile to cmake and ran the CI workflows locally so I think next time they run, they should pass. Sorry about that. Also I noticed that git was using my work account, so I sorted that out too. |
4bc4b04 to
2621d76
Compare
danmar
left a comment
There was a problem hiding this comment.
I am not sure why but there was significant slowdown in CI (selfchecking) after this?
|
Thanks for the review! I will look into it more on monday. |
|
I found the issue with the selfcheck slowdown. isAnonymousFunctionCall would sometimes link() backwards causing the checker to loop forever. |
|
the sanitizers/build check failed because of some network thing: Thanks! |
|
@aadanen I can't retrigger it now. maybe it's too old. can you please make a new commit to retrigger the CI |
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
New problem: a lost deallocuse warning. On the merge-base, passing a freed pointer to a call through a function pointer (free(p); (*cb)(p);) or to the result of a call (free(p); get()(p);) gives deallocuse. With this PR the warning is gone. The new branches set tok = lpar->link() (and return anonOpeningPar->link()), so the arguments never reach the dealloc‑use check in checkTokenInsideExpression. Calls through plain names (foo(p), cb(p)) do not warn on the merge-base either. So this warning was specific to these call forms, but it was a real use-after-free detection and the PR drops it silently.
The FP is only partly fixed. It is fixed for (*cb)(p), (s->fp)(p), (*s.fp)(p), (*fps[0])(p), (c ? a : b)(p) and get()(p). It remains for function-pointer calls through a subscript, such as fps[0](p) and s->fps[1](p) (dispatch tables), and for chained calls like get2()()(p).
I found no crash, hang or slowdown. I also found no other result changes on the cfg test files.
Testing
- I built
cppcheckandtestrunnerinpr/, andcppcheckinbase/. ./testrunner TestLeakAutoVar TestLeakAutoVarStrcpy TestLeakAutoVarRecursiveCountLimit TestLeakAutoVarWindows TestLeakAutoVarPosixran 161 tests with 0 failures.- I ran base and PR on
test/cfg/{posix,std,gnu}.c,test/cfg/{std,windows}.cppandlib/checkleakautovar.cpp. The output was identical, and the run time was the same (215s vs 214s). - I tried about 40 small cases: function pointers, lambdas (including immediately invoked ones), functors, casts,
sizeof, conditions, assignments, and nested or chained calls. The intended cases improve. There were no new false positives besides the one already raised in comments.md.
Regression: deallocuse is lost (c1.c)
#include <stdlib.h>
typedef void (*fn_t)(void*);
fn_t get(void);
void u1(void (*cb)(void*)) {
char *p = malloc(10);
free(p);
(*cb)(p);
}
void u3(void) {
char *p = malloc(10);
free(p);
get()(p);
}Command: cppcheck -q --template='{line}:{column}:{id}' c1.c
- base:
7:11:deallocuseand12:11:deallocuse - PR: no output
FPs that remain (d1.c)
#include <stdlib.h>
typedef void (*fn_t)(void*);
typedef fn_t (*fn2_t)(void);
fn2_t get2(void);
struct S { fn_t fps[2]; };
void k1(fn_t *fps) { char *p = malloc(10); fps[0](p); }
void k4(struct S *s) { char *p = malloc(10); s->fps[1](p); }
void k5(void) { char *p = malloc(10); get2()()(p); }The PR still reports memleak for all three functions, the same as base. Base and PR agree on these cases; the PR only fixes the parenthesised forms.
Automated review of cd4f032 by Claude. The pull request was built and tested locally.
|
Test results for commit cd4f032 (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8813/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
|
Test results for commit a0fa6fb (tools/test-my-pr.py, main compared to this PR): Test: http://ec2-16-170-140-253.eu-north-1.compute.amazonaws.com/pr-8813/ Posted automatically by the cppcheck PR test runner. +N: warnings only with this PR, -N: warnings only with main. The AI review is written by Claude and can be wrong. |
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems in the current head (a0fa6fb). The earlier reviews already raised the problems I know of: the lost deallocuse warning, the deallocuse false positive for (*cb)(fclose(f)), and the remaining false positives for chained calls. The latest commit also handles subscript calls such as fps[0](p), v[0](p), m["k"](p) and this->fps[0](p). Things that look like these calls but are not did not lose any real leak warnings: casts to unknown types, a[i] < 0, if (a[i]), (p)[0], new S[n]() and delete [] (p).
Testing
- Build:
cppcheckandtestrunnerbuilt inpr/, andcppcheckbuilt inbase/, without errors. - Unit tests:
./testrunner TestLeakAutoVarran 149 tests with 0 failures (10 todos). - Comparison with base: I wrote small files in
scratch/and ran both versions withcppcheck -q --template='{line}:{column}:{id}:{message}'.- Subscript calls (
t3.cpp): the PR removes thememleakfalse positive forv[0](p)with astd::vector<fn>, form["k"](p)with astd::map, and forthis->fps[0](p). - Leaks that must still be found (
t3.cpp): the PR still reportsmemleakfora[0](NULL),(*g)(1)(p),int x = a[0] < 0 ? 1 : 2;,if (a[i]) {}anda[i] = (p)[0];. It still reportsdoubleFreefora[0](p); free(p); free(p);. - Casts and parentheses (
t2.c): casts to unknown types such as(mylong_t)(p[0]),(MYVOID)(p)and(u8)(*p) + 1, andif (a) (p)[0] = 1;, give the same leaks on base and PR. new,deleteand functors (t1.cpp):new char[n]()anddelete [] (p); p[0]=1;give the same results on base and PR.A<int>()(p)(a templated functor) no longer gives amemleak, as intended. Thedeallocusewarning forfree(p); (g)(p);with a lambda is lost, which was already reported.- Conditions and returns (
t4.cpp):if ((*cb)(p) < 0) return 1;no longer gives the false positive.return (*cb)(NULL);andint r = (*cb)(NULL);still report the leak.
- Subscript calls (
- cfg files: I ran base and PR on
test/cfg/{qt,wxwidgets,boost,windows}.cppandtest/cfg/gtk.c, with the matching--libraryand--check-library --enable=information. The output was identical, with no crashes or hangs.
Automated review of a0fa6fb by Claude. The pull request was built and tested locally.
dmcppcheck
left a comment
There was a problem hiding this comment.
AI review
I found no new problems that would block this PR. The new handling of multidimensional arrays from C20 works: a[0][1](p), a[0][1][1](p) and s->f[1][0](p) no longer give the memleak false positive, and the real leaks, doubleFree and deallocuse warnings that I checked still match base.
One small new gap, of the same kind as the chained calls already raised in C16: a function call followed by a subscript, such as g()[0](p), still gives the memleak false positive. The new while loop only steps over ] [, not ) [.
The earlier problems are unchanged: the lost deallocuse warning (C15), the remaining chained-call false positive (C16) and the (*cb)(fclose(f)) false positive (C11), which the author says base also has for plain calls.
Earlier comments
| # | Comment | By | Where | Status |
|---|---|---|---|---|
| 1 | Redundant link() check on parenthesis token | @danmar | lib/checkleakautovar.cpp:295 |
✅ Fixed |
| 2 | Selfcheck slowdown in CI | @danmar | general | ✅ Fixed |
| 3 | deallocuse FP for (*cb)(fclose(f)) and get_fn()(fclose(f)) | @danmar | lib/checkleakautovar.cpp:844 |
💬 Answered: Base gives the same FP for plain calls like foo(fclose(f)); the author wants a separate PR for it |
| 4 | deallocuse lost for free(p); (*cb)(p); and get()(p) | AI review | general | ☑️ Resolved, code unchanged |
| 5 | FP remains for subscript calls and chained calls | AI review | general | ☑️ Resolved, code unchanged |
| 6 | Arguments skipped, so deallocuse is no longer checked | AI review | lib/checkleakautovar.cpp:847 |
☑️ Resolved, code unchanged |
| 7 | Subscript and chained calls like get2()()(p) are not recognised | AI review | lib/checkleakautovar.cpp:284 |
☑️ Resolved, code unchanged |
| 8 | Handle multidimensional arrays | @danmar | lib/checkleakautovar.cpp:298 |
✅ Fixed |
Testing
- Build:
cppcheckandtestrunnerbuilt inpr/, andcppcheckbuilt inbase/, without errors. - Unit tests:
./testrunner TestLeakAutoVarran 150 tests with 0 failures (10 todos). - Comparison with base: I ran both versions with
cppcheck -qand a template that prints line, column and id, on small files inscratch/.- Multidimensional arrays: the PR removes the false positive for
a[0][1](p),a[0][1][1](p)ands->f[1][0](p). - Leaks that must still be found: the PR still reports
memleakfora[0][1](NULL),int x = a[0][1];andint x = a[0][1](1);. delete [] (p): followed by a second delete it still givesdoubleFree, and followed by a use it still givesdeallocuse.- Keywords before a parenthesised call:
if,while,for,switch,doandreturnfollowed by(cb)(NULL)give the same results as base. - Lambdas:
return [p](){...};,return [](){...}();andthrow [](){...}();give the same results as base.
- Multidimensional arrays: the PR removes the false positive for
False positive that remains (new form)
#include <cstdlib>
typedef void (*fn_t)(void*);
fn_t* g();
void m6() { char *p = (char*)malloc(10); g()[0](p); }Base and PR both report memleak for m6 (line 4, column 53).
Automated review of ccadb4c by Claude. The pull request was built and tested locally.
Modify checkleakautovar to consider calls to lambdas and function pointers as function calls that can allow resources to escape the current scope.